Skip to content

Add a [pulse] built-in effect to RichTextLabel - #77117

Merged
YuriSizov merged 1 commit into
godotengine:masterfrom
Calinou:richtextlabel-add-pulse-effect
Jul 12, 2023
Merged

Add a [pulse] built-in effect to RichTextLabel#77117
YuriSizov merged 1 commit into
godotengine:masterfrom
Calinou:richtextlabel-add-pulse-effect

Conversation

@Calinou

@Calinou Calinou commented May 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

In games, blinking text is one of the more frequently used animations. It can be (sparingly) used to bring attention to important messages in a chat log or inventory tooltip, for instance.

This effect accepts the following options:

  • freq: How fast text blinks (higher is faster).
  • color: The target color multiplier for blinking. The default mostly fades out text, but not entirely (for better accessibility).
  • ease: The easing function exponent to use. Negative values provide in-out easing, which is why -2.0 is the default.

Testing project: test_richtexteffect_pulse.zip
Note: Opening the project will print this error about 100 times, but this occurs when using unmodified master branch too:

ERROR: Condition "p_ptr == nullptr" is true.
   at: free_static (core/os/memory.cpp:148)

Preview

C++-based version (this PR) on the left, script-based version on the right (included in the testing project):

test_richtexteffect_pulse.mp4

@Calinou
Calinou requested a review from a team as a code owner May 15, 2023 23:18
@Calinou Calinou added this to the 4.x milestone May 15, 2023
@Calinou Calinou mentioned this pull request May 16, 2023
Open
@Calinou
Calinou force-pushed the richtextlabel-add-pulse-effect branch from d74e78b to c48847e Compare May 16, 2023 22:44
@mhilbrunner

mhilbrunner commented May 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Seems useful! Thoughts:

  1. Would it be possible to add a parameter for total number of pulses before it stops? With the default/-1 being infinite, but its pretty common to let an important keyword pulse once (or 2-3 times) only to avoid it being distracting. Only if this won't add a ton of complexity though.
  2. At first glance, it feels a bit weird for the bobbing effect to be a pulse effect parameter instead of its own thing, but maybe thats just me.

@deralmas

deralmas commented May 21, 2023

Copy link
Copy Markdown
Member

@mhilbrunner

At first glance, it feels a bit weird for the bobbing effect to be a pulse effect parameter instead of its own thing, but maybe thats just me.

Yeah I agree with your thoughts and especially this one.

@Calinou

Calinou commented May 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author
  1. Would it be possible to add a parameter for total number of pulses before it stops? With the default/-1 being infinite, but its pretty common to let an important keyword pulse once (or 2-3 times) only to avoid it being distracting. Only if this won't add a ton of complexity though.

To do this, I need to check if it's possible to set a state variable when the RichTextLabel is last modified.

  1. At first glance, it feels a bit weird for the bobbing effect to be a pulse effect parameter instead of its own thing, but maybe thats just me.

I'm not sure if a dedicated tag for bobbing is warranted, but I may as well remove the bobbing functionality entirely as I don't think it'll be used often.

@Calinou

Calinou commented May 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

I've pushed a commit that removes the height option from the [pulse] effect, in case we decide to go with that route.

@mhilbrunner

mhilbrunner commented May 23, 2023

Copy link
Copy Markdown
Member

To do this, I need to check if it's possible to set a state variable when the RichTextLabel is last modified.

Don't spent too much time on it, can also be followup work. Would be useful though.

I've pushed a commit that removes the height option from the [pulse] effect, in case we decide to go with that route.

Makes sense IMO

@Calinou
Calinou force-pushed the richtextlabel-add-pulse-effect branch from f511811 to aa68fb2 Compare May 31, 2023 11:05
@KoBeWi

KoBeWi commented Jun 1, 2023

Copy link
Copy Markdown
Member

Would it be possible to add a parameter for total number of pulses before it stops?

This wouldn't work well with text gradually appearing with visible characters.

KoBeWi
KoBeWi approved these changes Jun 1, 2023
View reviewed changes

@KoBeWi KoBeWi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks ok and it works correctly.

@akien-mga

akien-mga commented Jun 2, 2023

Copy link
Copy Markdown
Member

CC @Eoin-ONeill-Yokai, you might be interested ;)

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jun 2, 2023
@akien-mga

akien-mga commented Jun 2, 2023

Copy link
Copy Markdown
Member

I think we're close to consensus so I'm putting this on the 4.2 milestone (too late for 4.1 as we entered feature freeze).

mhilbrunner
mhilbrunner approved these changes Jun 2, 2023
View reviewed changes

@mhilbrunner mhilbrunner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature looks good to me, same for the code. Will need to be documented.

Eoin-ONeill-Yokai
Eoin-ONeill-Yokai approved these changes Jun 2, 2023
View reviewed changes

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I don't see any problems with the code.

I'm happy to see this integrated directly in the engine if others are; I admittedly only restricted it to a custom RichTextEffect implementation only as a proof of concept to make sure that the API was good enough as I had originally implemented it into the C++ directly. There's a discussion to be had on whether or not the custom RTEs could be less of a burden for users to use, but I don't think a decision should be made there until we really flesh out our asset library situation (as discussed in the sprint right now ;) )

Anyway, looks good and works as expected.

@djrain

djrain commented Jun 4, 2023 •
edited
Loading

Copy link
Copy Markdown

The "ghost" effect in the docs is very nice and quite similar - seems to me that its "span" variable could fit in nicely here, and make it a lot more flexible. Can we combine them?

@Calinou

Calinou commented Jun 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

The "ghost" effect in the docs is very nice and quite similar - seems to me that its "span" variable could fit in nicely here, and make it a lot more flexible. Can we combine them?

This doesn't seem to be something commonly used in games. We have a custom effect system so you can still use it 🙂

Either way, I've squashed commits in this PR and rebased it against master (no other changes were made).

@Calinou
Add a [pulse] built-in effect to RichTextLabel
70e6c3c 
In games, blinking text is one of the more frequently used animations.
It can be (sparingly) used to bring attention to important messages
in a chat log or inventory tooltip, for instance.

This effect accepts the following options:

- `freq`: How fast text blinks (higher is faster).
- `color`: The target color multiplier for blinking.
  The default mostly fades out text, but not entirely (for better accessibility).
- `ease`: The easing function exponent to use.
  Negative values provide in-out easing, which is why `-2.0` is the default.
@Calinou
Calinou force-pushed the richtextlabel-add-pulse-effect branch from aa68fb2 to 70e6c3c Compare June 5, 2023 16:45
@YuriSizov
YuriSizov merged commit 7550b02 into godotengine:master Jul 12, 2023
@YuriSizov

YuriSizov commented Jul 12, 2023

Copy link
Copy Markdown
Contributor

Thanks!

@Calinou
Calinou deleted the richtextlabel-add-pulse-effect branch July 12, 2023 17:38
@Calinou Calinou mentioned this pull request Oct 25, 2023
Merged
@Calinou Calinou mentioned this pull request Jul 15, 2026
Open
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
@YuriSizov
Merge pull request godotengine#77117 from Calinou/richtextlabel-add-p…
e73ea0d 
…ulse-effect

Add a `[pulse]` built-in effect to RichTextLabel
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
@YuriSizov
Merge pull request godotengine#77117 from Calinou/richtextlabel-add-p…
4b752ae 
…ulse-effect

Add a `[pulse]` built-in effect to RichTextLabel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhilbrunner mhilbrunner mhilbrunner approved these changes
@KoBeWi KoBeWi KoBeWi approved these changes
+1 more reviewer
@Eoin-ONeill-Yokai Eoin-ONeill-Yokai Eoin-ONeill-Yokai approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement topic:gui

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

Add a [pulse] built-in effect to RichTextLabel

8 participants

@Calinou @mhilbrunner @deralmas @KoBeWi @akien-mga @djrain @YuriSizov @Eoin-ONeill-Yokai